Improve Apple container compose compatibility#119
Conversation
thromel
force-pushed
the
fix/apple-container-compose-compat
branch
from
b0b30a4 to
1ee22f2
Compare
thromel
changed the title
thromel
force-pushed
the
fix/apple-container-compose-compat
branch
from
c46f218 to
d862ae7
Compare
Cyb3rDudu
left a comment
Cyb3rDudu
left a comment
There was a problem hiding this comment.
Great PR - this meaningfully closes the gap with Apple container, and the parsing/arg-building test coverage is solid. I'd like to merge this, but I think two items should be addressed first since they're regressions on existing workflows rather than missing features. The rest can be follow-ups.
Blockers (before merge)
- up now hard-fails on transitive dependencies
Sources/Container-Compose/Commands/ComposeUp.swift:160
The service filter only keeps a service if it's requested or a direct dependent (via dependedBy, which topoSortConfiguredServices populates one level deep). For a chain c → b → a, compose up c keeps c and b but drops a. The new waitForDependencyConditions then throws dependencyNotStarted("b", "a"). Before this PR the missing grandparent was tolerated, so this is a regression on a core workflow.
Fix: expand the filter to keep the full transitive dependency closure of the requested services (walk depends_on recursively), or have service_started start a missing dependency on demand instead of throwing.
- up -d silently reports success when a long-running service crashes
Sources/Container-Compose/Commands/ComposeUp.swift:336
container run -d returns exit 0 on backgrounding, so the guard at :726 passes. If the container then crashes within the poll window, it reaches .stopped and waitUntilServiceStarted records .completed — the container's real exit code is never inspected. This undermines the PR's own goal of surfacing failed detached runs. A db that exits non-zero ~1s after start will print success and launch dependents against a dead service.
Fix: for a .stopped long-running service, inspect the container's actual exit status rather than assuming success.
Follow-ups (file as issues, don't block)
- retries is total attempts, not consecutive failures (ComposeUp.swift:797) — a healthy-but-slow service (e.g. a DB needing ~45s) is declared unhealthy. Compose treats retries as consecutive failures.
- healthcheck.timeout is parsed but never applied (ComposeUp.swift:806) — a hanging probe blocks streamCommand indefinitely instead of being bounded.
- service_completed_successfully race in attached mode (ComposeUp.swift:730) — the detached Task + immediate poll can sample a one-shot as .running before it exits, breaking its dependents. Timing-dependent; worth a known-limitation note.
Minor (cleanup)
- supportsNetworkAliases() is re-evaluated on every networkRunArg call (:295) — cache it in a static let.
- Healthcheck.parseDuration recompiles the regex on every call (Healthcheck.swift:58) — hoist to static let.
- The detach/attached branches duplicate handleOutput, the start banner, and the streamCommand call (:716) — extract the shared part and branch only on error handling.
Happy to help with any of these. The core work here is solid - just want to land it without the two regressions.
|
Separate from the items in the review above - wanted to flag one thing on the volume handling specifically. The native-volume switch is the right call, and I like that it respects Coincidentally #122 just opened and hits this same branch from the other direction: it uses let actualVolumeName = volumeConfig.name
?? volumeConfig.external?.name // external: keep as-is, skip create
?? "\(projectName)_\(source)" // fall back to project-namespacedThat keeps everything you've built and adds the isolation. Happy to help wire it up. |
thromel
force-pushed
the
fix/apple-container-compose-compat
branch
from
d862ae7 to
bdc0a64
Compare
|
Thanks for the detailed review. I pushed |
2 participants
Summary
This improves compatibility with Apple
containerfor several Compose workflows:depends_onmap form and preserve dependency conditionsservice_started,service_healthy, andservice_completed_successfullynetworksobject form, including aliasescontainercommand parser supports them, while preserving a warning/skip fallback for Apple Container 1.0.0hostname:because Applecontainer run1.0.0 does not currently expose a hostname flag; proposed upstream support remains under discussion in Add container run hostname flag apple/container#1811CMDandCMD-SHELL) with interval/start-period/retry handlingcontainer volumesupport instead of rewriting them into host pathscontainer runcommands as errors, which makes one-shot failures failcompose upRelated issues: #52, #115, #116, #117, and tracker #118.
Apple-side service discovery work is tracked upstream in apple/container#1809. Current upstream PR chain:
container run --hostname, signed and under maintainer discussion; aliases may cover most Compose cases--network name,alias=..., signed, closes [Request]: Add network-scoped aliases for container network attachments apple/container#1839Testing
Static/build checks:
The static test command passed 141 tests.
WaitForeverCpuTestswas skipped because the existing CPU-threshold test is noisy on this machine and unrelated to these changes.Manual Apple
containersmoke tests using the locally builtContainer-Composebinary:up -dcontainer runhostname:emits a warning, does not pass unsupported--hostname, and completes successfully (HOSTNAME_OKlogged)All dynamic test containers, networks, and volumes were cleaned up after the runs.